Code Review: Mock.HttpClientFactory() helper

The feature is clean and addresses a real UX pain point well. The API shape is consistent with the existing Mock.HttpHandler() / Mock.HttpClient() patterns. A few concerns worth discussing before merge:

1. MockHttpClientFactory should implement IDisposable

MockHttpClientFactory creates and owns MockHttpHandler instances (both the default handler and any named handlers added via WithHandler). MockHttpHandler extends HttpMessageHandler, which is IDisposable. Not cleaning up owned disposable resources is a resource leak, even if MockHttpHandler currently has no unmanaged handles — a future MockHttpHandler implementation might.

For symmetry, note that the real DefaultHttpClientFactory in Microsoft.Extensions.Http also implements IDisposable.

Suggested fix:

public sealed class MockHttpClientFactory : IHttpClientFactory, IDisposable
{
    private bool _disposed;

    public void Dispose()
    {
        if (_disposed) return;
        _disposed = true;
        Handler.Dispose();
        foreach (var h in _named.Values)
            h.Dispose();
    }
}

2. _named dictionary isn't thread-safe

MockHttpHandler carefully uses Lock for _setups and ConcurrentQueue for _requests. The _named dictionary in MockHttpClientFactory doesn't follow the same discipline — concurrent WithHandler calls (e.g. from parallel test setup helpers) will race.

A ConcurrentDictionary is the simplest fix and keeps the pattern consistent:

private readonly ConcurrentDictionary<string, MockHttpHandler> _named = new(StringComparer.Ordinal);

WithHandler can then use _named[name] = handler (or TryAdd if duplicate names should be rejected rather than silently overwritten).

3. No test for CreateClient("") / the unnamed default client

The most common real-world call site is factory.CreateClient(string.Empty) (i.e., IHttpClientFactory is called with HttpClient.DefaultName). The current tests only exercise named strings like "any", "default", "x". Adding a test with string.Empty ensures the fallback path works for the most common usage.

4. Minor: MockHttpHandler.CreateClient() vs MockHttpClientFactory.CreateClient()

MockHttpHandler.CreateClient() (line 22) creates a disposing client (new HttpClient(this)). MockHttpClientFactory.CreateClient() creates a non-disposing one (disposeHandler: false). These two paths have different disposal semantics from the same handler. The inconsistency is subtle — worth either a comment on MockHttpHandler.CreateClient() noting the disposal difference, or aligning them.

Overall

The core implementation is well-scoped and the docs section is excellent. The IDisposable omission is the only item I'd call a genuine defect (owned disposable resources that are never cleaned up); the rest are polish. Happy to see this land once that's addressed.

Follow-up Review: refactor(mocks) commit (62173e5)

Thanks for the second commit — the WithBaseAddress overloads are a welcome ergonomic addition and the case-insensitive name test is the right call. Here's the status of each item from the first review:

Carried forward: still missing (was: issue 1)

MockHttpClientFactory still creates and owns MockHttpHandler instances that are never disposed. Nothing changed here. MockHttpHandler extends HttpMessageHandler : IDisposable, and for named handlers passed via WithHandler, the factory signals ownership implicitly by storing them. The real DefaultHttpClientFactory implements IDisposable for the same reason.

Carried forward: still not thread-safe (was: issue 2)

The OrdinalIgnoreCase comparer was added (good — it matches IHttpClientFactory semantics), but the collection is still a plain Dictionary, not a ConcurrentDictionary. MockHttpHandler uses Lock and ConcurrentQueue throughout. Keeping the same discipline here prevents subtle races if tests ever call WithHandler from parallel setup:

Carried forward: no CreateClient(string.Empty) test (was: issue 3)

All tests still use "test-client". The most common real-world call path is factory.CreateClient(string.Empty) (i.e., HttpClient.DefaultName), which many DI containers inject. A one-liner test would confirm the fallback works for that path:

Resolved: disposal inconsistency (was: issue 4)

The base-address ergonomics are now handled at the factory level (WithBaseAddress), so users won't need to reach through to MockHttpHandler.CreateClient() as a workaround. The inconsistency still exists technically, but it's no longer a practical concern. Marking this resolved.

Summary

Two genuine defects remain: missing IDisposable (owned disposable resources never released) and a plain Dictionary in a concurrent context. The empty-string test is minor but low-effort. Happy to approve once those two are fixed.

Follow-up Review: refactor(mocks) commit (62173e5)

Thanks for the second commit — the WithBaseAddress overloads are a welcome ergonomic addition and the case-insensitive name test is the right call. Here's the status of each item from the first review:

Carried forward: IDisposable still missing (was: issue 1)

MockHttpClientFactory still creates and owns MockHttpHandler instances that are never disposed. Nothing changed here. MockHttpHandler extends HttpMessageHandler : IDisposable, and for named handlers passed via WithHandler, the factory signals ownership implicitly by storing them. The real DefaultHttpClientFactory implements IDisposable for the same reason.

Suggested fix (same as before):

public sealed class MockHttpClientFactory : IHttpClientFactory, IDisposable
{
    private bool _disposed;

    public void Dispose()
    {
        if (_disposed) return;
        _disposed = true;
        Handler.Dispose();
        foreach (var h in _named.Values)
            h.Dispose();
    }
}

Carried forward: _named still not thread-safe (was: issue 2)

The OrdinalIgnoreCase comparer was added (good — it matches IHttpClientFactory semantics), but the collection is still a plain Dictionary, not a ConcurrentDictionary. MockHttpHandler uses Lock and ConcurrentQueue throughout. Keeping the same discipline here prevents subtle races if tests ever call WithHandler from parallel setup:

private readonly ConcurrentDictionary<string, MockHttpHandler> _named =
    new(StringComparer.OrdinalIgnoreCase);

Carried forward: no CreateClient(string.Empty) test (was: issue 3)

All tests still use "test-client". The most common real-world call path is factory.CreateClient(string.Empty) (i.e., HttpClient.DefaultName), which many DI containers inject. A one-liner test would confirm the fallback works for that path:

[Test]
public async Task CreateClient_EmptyNameFallsBackToDefaultHandler()
{
    var factory = Mock.HttpClientFactory().WithBaseAddress("http://localhost");
    factory.Handler.OnAnyRequest().Respond(HttpStatusCode.OK);
    using var client = factory.CreateClient(string.Empty);
    var response = await client.GetAsync("/");
    await Assert.That(response.StatusCode).IsEqualTo(HttpStatusCode.OK);
}

Resolved: disposal inconsistency (was: issue 4)

The base-address ergonomics are now handled at the factory level (WithBaseAddress), so users won't need to reach through to MockHttpHandler.CreateClient() as a workaround. The inconsistency still exists technically, but it's no longer a practical concern. Marking this resolved.

Summary

Two genuine defects remain: missing IDisposable (owned disposable resources never released) and a plain Dictionary in a concurrent context. The empty-string test is minor but low-effort. Happy to approve once those two are fixed.

Follow-up Review (third pass)

The diff in this pass is the snapshot reordering commit (dc70791) — no changes to MockHttpClientFactory or tests. All three outstanding items from the previous two reviews remain unresolved.

Still open: IDisposable missing (issue 1, raised twice)

MockHttpClientFactory creates and owns a MockHttpHandler (which extends HttpMessageHandler : IDisposable) and stores named handlers passed via WithHandler. Nothing disposes them. This is the same defect flagged in both prior rounds.

public sealed class MockHttpClientFactory : IHttpClientFactory, IDisposable
{
    private bool _disposed;

    public void Dispose()
    {
        if (_disposed) return;
        _disposed = true;
        Handler.Dispose();
        foreach (var h in _named.Values)
            h.Dispose();
    }
}

Still open: _named not thread-safe (issue 2, raised twice)

The OrdinalIgnoreCase comparer was added in the second commit (good), but it is still a plain Dictionary. MockHttpHandler consistently uses Lock and ConcurrentQueue. The same discipline should apply here:

private readonly ConcurrentDictionary<string, MockHttpHandler> _named =
    new(StringComparer.OrdinalIgnoreCase);

Still open: no CreateClient(string.Empty) test (issue 3, raised twice)

All tests still use "test-client". The most common real-world call is factory.CreateClient(string.Empty) (HttpClient.DefaultName), which is what most DI containers use. A one-liner would cover it:

[Test]
public async Task CreateClient_EmptyNameFallsBackToDefaultHandler()
{
    var factory = Mock.HttpClientFactory().WithBaseAddress("http://localhost");
    factory.Handler.OnAnyRequest().Respond(HttpStatusCode.OK);
    using var client = factory.CreateClient(string.Empty);
    var response = await client.GetAsync("/");
    await Assert.That(response.StatusCode).IsEqualTo(HttpStatusCode.OK);
}

Summary

The feature is well-designed and the docs are solid. The two genuine defects (IDisposable and thread-safety) have now been flagged three times without action — would appreciate a response indicating whether these are intentional omissions or just pending work. Happy to approve once the IDisposable gap is closed and _named is a ConcurrentDictionary.

Code Review

Solid implementation that cleanly solves the ObjectDisposedException UX pain from #5885. The design is focused, the thread safety is well-handled, and the API is consistent with the existing MockHttpClient patterns.

One correctness gap: named base address is untested

WithBaseAddress(string name, string baseAddress) stores into _baseAddresses, and CreateClient uses it on line 64:

var baseAddress = _baseAddresses.TryGetValue(name, out var named) ? named : _defaultBaseAddress;

But no test calls factory.CreateClient("users") and asserts client.BaseAddress. The existing tests only verify handler routing via HandlerFor(). A simple addition would cover this:

[Test]
public async Task CreateClient_UsesNamedBaseAddressWhenRegistered()
{
    var namedHandler = Mock.HttpHandler();
    using var factory = Mock.HttpClientFactory()
        .WithHandler("users", namedHandler)
        .WithBaseAddress("users", "https://users.example.com");

    using var client = factory.CreateClient("users");

    await Assert.That(client.BaseAddress).IsEqualTo(new Uri("https://users.example.com"));
}

Minor: _disposed read is not memory-safe

ThrowIfDisposed() reads _disposed without a fence, while Dispose() writes it via Interlocked.Exchange. The JIT can legally cache the read in a register, meaning a disposed factory could appear live on another thread. The fix follows the BCL pattern (e.g. CancellationTokenSource):

private volatile int _disposed;

Or alternatively Volatile.Read(ref _disposed) in ThrowIfDisposed. For a test helper the risk is low, but it's a trivial fix.

The unrelated snapshot change is expected

The ClassTimelineAttribute reordering in Tests.Core_Library_Has_No_API_Changes.Net4_7.verified.txt is just alphabetical re-sorting from the merge conflict resolution — no concern there.

What's done well:

• disposeHandler: false is exactly the right fix for the core UX problem
• Interlocked.Exchange makes Dispose() idempotent with no double-dispose risk
• The HashSet<MockHttpHandler> deduplication in Dispose() correctly handles the same handler registered under multiple names
• Case-insensitive StringComparer.OrdinalIgnoreCase on both dictionaries matches IHttpClientFactory semantics
• Fluent builder returns this consistently, enabling the chain in tests
• 9 test cases cover the happy paths, disposal, fallback, and multi-lifetime verification well
• Docs section is clear and the before/after examples in the PR description are helpful

The named base address test gap is the only thing I'd want to see filled before merge. The volatile fix is worth doing but not blocking.